Set default subscription values when not specified #228
Conversation
| if (subSpec.CatalogSource != "" && subSpec.CatalogSource != catalog) || | ||
| (subSpec.CatalogSourceNamespace != "" && subSpec.CatalogSourceNamespace != catalogNamespace) { | ||
| return errors.New( | ||
| "the subscription defaults could not be determined because the catalog doesn't match those in the " + | ||
| "PackageManifest", | ||
| ) | ||
| } |
There was a problem hiding this comment.
For a minute, I was thinking that this kind of message could be good to surface regardless of defaulting. But I expect that the subscription would give some kind of error if the catalog doesn't match the package, so the user would still get a message there. Do you think that's right?
| "the subscription defaults could not be determined because the catalog doesn't match those in the " + | ||
| "PackageManifest", |
There was a problem hiding this comment.
| "the subscription defaults could not be determined because the catalog doesn't match those in the " + | |
| "PackageManifest", | |
| "the subscription defaults could not be determined because the catalog specified in the " + | |
| "policy does not match what was found in the PackageManifest on the cluster", |
Especially if the user is not familiar with OLM, I think we should give some extra hints on where these values came from.
test/resources/case38_operator_install/operator-policy-all-defaults.yaml
Show resolved
Hide resolved
mprahl
force-pushed
the
operator-defaults2
branch
from
077037a
to
c35614c
Compare
| if err != nil { | ||
| if k8serrors.IsNotFound(err) { | ||
| return fmt.Errorf( | ||
| "%wthe subscription defaults could not be determined because the PackageManifest was not found", |
There was a problem hiding this comment.
I don't know if I've seen wrapping a specific empty error like this. It's interesting. I'm still trying to find a way to do this kind of thing that I really like, did you find this somewhere else (just for reference)?
There was a problem hiding this comment.
We do this in the propagator with ErrRetryable but that comes from me, so I don't know of prior art doing this but I think it does the trick. I think the more "right" thing would be to have a custom error type as opposed to wrapping a custom error, but I didn't think it was worth the extra code.
| if catalog == "" || catalogNamespace == "" { | ||
| return fmt.Errorf( | ||
| "%wthe subscription defaults could not be determined because the PackageManifest didn't specify a catalog", | ||
| ErrPackageManifest, | ||
| ) | ||
| } |
There was a problem hiding this comment.
I'm not sure that this one should wrap the error so it's retried - I think usually this would get fixed by the content in the catalog changing? What was your reasoning
There was a problem hiding this comment.
I don't know that this would ever occur, but I figured it was likely due to partial results being returned but it seems unlikely. I'll remove the retry.
| if defaultChannel == "" { | ||
| return fmt.Errorf( | ||
| "%wthe default channel could not be determined because the PackageManifest didn't specify one", | ||
| ErrPackageManifest, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Same here, I'm not sure if this should be retried.
This sets the default channel, source, and sourceNamespace when not provided in the policy. Relates: https://issues.redhat.com/browse/ACM-10561 Signed-off-by: mprahl <mprahl@users.noreply.github.com>
mprahl
force-pushed
the
operator-defaults2
branch
from
c35614c
to
8211f51
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JustinKuli, mprahl The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit e71a482
into
open-cluster-management-io:main
This sets the default channel, source, and sourceNamespace when not provided in the policy.
Relates:
https://issues.redhat.com/browse/ACM-10561